Skip to content

geotiff: drop dead bytearray allocation in uncompressed tiled writer (#1736)#1746

Merged
brendancol merged 2 commits into
mainfrom
issue-1736
May 12, 2026
Merged

geotiff: drop dead bytearray allocation in uncompressed tiled writer (#1736)#1746
brendancol merged 2 commits into
mainfrom
issue-1736

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • _compress_tiles uncompressed branch allocated total_buf = bytearray(n_tiles * tile_bytes) and a memoryview, then built every tile via tile_arr.tobytes() and never read the buffer back.
  • The buffer was roughly the size of the full uncompressed raster, so a marginal write was held alongside the actual tile list. Removing it.
  • Round-trip behaviour unchanged.

Closes #1736.

Test plan

  • New test_writer_uncompressed_tiled_no_dead_alloc_1736.py pins three uncompressed-tiled round-trips: exact-divisor tile size, edge-tile padding, and multi-band.
  • Existing writer test suite still passes (239 tests).

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 12, 2026
@brendancol brendancol requested a review from Copilot May 12, 2026 20:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes an unused large pre-allocation in the GeoTIFF CPU writer’s uncompressed tiled path, preventing unnecessary peak-memory growth during writes. It also adds a regression test module that exercises uncompressed tiled round-trips across several tile/shape configurations.

Changes:

  • Remove the dead bytearray(n_tiles * tile_bytes) allocation (and associated memoryview) in the uncompressed tiled writer path.
  • Add regression tests covering uncompressed tiled round-trips for exact-division tiles, padded edge tiles, and multiband data.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
xrspatial/geotiff/_writer.py Deletes the unused uncompressed-tile pre-allocation and replaces it with explanatory comments.
xrspatial/geotiff/tests/test_writer_uncompressed_tiled_no_dead_alloc_1736.py Adds regression round-trip coverage for uncompressed tiled writes (including edge padding and multiband).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +3 to +8
The uncompressed tiled-write branch of ``_compress_tiles`` previously
allocated a contiguous ``bytearray`` plus a memoryview ``(n_tiles *
tw * th * bytes_per_sample * samples)`` bytes long at the top of the
loop and never read either back. Tile bytes were still built via
``tile_arr.tobytes()`` and appended to a list. The dead buffer roughly
doubled peak memory for an uncompressed write.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 29ebb71. The module docstring now points at xrspatial.geotiff._writer._write_tiled instead of the non-existent _compress_tiles.

Comment on lines +26 to +30
def test_uncompressed_tiled_round_trip_exact(tmp_path):
rng = np.random.RandomState(20260512)
h, w = 96, 144
data = rng.randint(0, 200, size=(h, w)).astype(np.uint8)
da = xr.DataArray(data, dims=['y', 'x'])
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 29ebb71 by adding the explicit allocation check, since the test file name promises one.

Two new tests (test_uncompressed_tiled_peak_memory_single_band and test_uncompressed_tiled_peak_memory_multiband) snapshot tracemalloc peak across a direct call to _write_tiled on the uncompressed branch and assert peak stays under 1.5x raster bytes. Calibration:

variant current peak with dead bytearray restored
1024x1024 uint8, tile_size=256 1.06x 2.07x
1024x1024x3 uint8, tile_size=256 1.06x 2.06x

1.5x leaves room for unrelated implementation drift but firmly catches the regression. I verified locally by monkey-patching _write_tiled to reintroduce the bytearray(n_tiles * tile_bytes) allocation; both new tests fail on the patched version with ratio ~2.06.

…1736)

The uncompressed branch of _compress_tiles allocated

    total_buf = bytearray(n_tiles * tile_bytes)
    mv = memoryview(total_buf)

at the top of the loop and never read either back. Tiles were still
built via tile_arr.tobytes() and appended to a list. The buffer was
roughly the size of the full uncompressed raster, so a marginal write
would OOM while holding both the dead buffer and the tile list.

Remove the dead allocation. Existing tile-build logic stays the same;
the round-trip is byte-for-byte unchanged.

Adds regression coverage in
test_writer_uncompressed_tiled_no_dead_alloc_1736.py for the exact
divisor, edge-tile padding, and multi-band paths.
Addresses two Copilot review comments on PR #1746:

* Update the module docstring's stale reference to ``_compress_tiles``
  (which doesn't exist) to point at the actual function name,
  ``xrspatial.geotiff._writer._write_tiled``.

* Add real teeth behind the "no dead alloc" test-module name. The
  three round-trip tests only covered correctness; nothing in the
  file would have failed if the ``bytearray(n_tiles * tile_bytes)``
  allocation were quietly restored.

Two new tests snapshot ``tracemalloc`` peak across a direct
``_write_tiled`` call (uncompressed branch, single- and 3-band 1024x1024
uint8 inputs) and assert peak stays under 1.5x raster bytes. The
current implementation lands at ~1.06-1.12x; the deleted bytearray
pushed peak to ~2.07x. The 1.5x cap reliably catches reintroduction
without flagging unrelated implementation drift.

The tracemalloc approach was preferred over renaming the tests/file
to "round-trip-only" because it preserves both the original intent
of the test name and gives future maintainers an automatic alarm if
the regression is reintroduced.
@brendancol brendancol merged commit f2db37a into main May 12, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

geotiff: uncompressed tiled writer pre-allocates a buffer it never uses

2 participants